ASoC: SOF: skl enable the core and get ROM init - #120
Conversation
lgirdwood
left a comment
lgirdwood
left a comment
There was a problem hiding this comment.
Overall needing more comments explaining what each block does.
There was a problem hiding this comment.
Is this really needed to set tag to 0?
There was a problem hiding this comment.
No, this is for debug, this function will be implement in next patch. and if not set a value, checkpatch will give a warning.
There was a problem hiding this comment.
Please always use C style comments /* comment */
There was a problem hiding this comment.
Just set tag = 0 at start of this function if it's always going to be 0 ???
There was a problem hiding this comment.
Like answered above, the next patch will get the true tag value, this is only for debug.
There was a problem hiding this comment.
structures should always be at the top of the variables.
There was a problem hiding this comment.
Btw, github can skew the formatting a little, so it's best to always run checkpatch.
There was a problem hiding this comment.
I have run the checkpatch, but don't know why here is still not aligned.
There was a problem hiding this comment.
this is just github rendering, if checkpatch is OK then you are good.
There was a problem hiding this comment.
pls comment what we are setting and why
There was a problem hiding this comment.
OK, so every register set need to be comment? Or only the main part?
There was a problem hiding this comment.
ideally, every register if its not obvious.
There was a problem hiding this comment.
Need to comment what this block does, and why we check core status here.
There was a problem hiding this comment.
OK, will have comment on that.
There was a problem hiding this comment.
I guess this is just for debug ?
There was a problem hiding this comment.
You will need to temporarily add an offset here to strip the extended manifest when using the closed source firmware (as it has an extended manifest pre pended ato the start of the FW binary file) otherwise the FW will not authenticate or boot..
There was a problem hiding this comment.
OK, will ask Keyon what is the offset. We still need to check if the firmware is rightly booted?
There was a problem hiding this comment.
yes, FW will not boot without the offset change and remove of extended manifest
0930255 to
69a810a
Compare
1. enable the skl core 2. get the ROM int Signed-off-by: Zhu Yingjiang <yingjiang.zhu@linux.intel.com>
69a810a to
2310452
Compare
lgirdwood
left a comment
lgirdwood
left a comment
There was a problem hiding this comment.
I'm assuming this PR is the correct one as it looks more complete than your other PR ? Can you close the old PR that way I know I'm reviewing the correct one.
| unsigned int size, struct snd_dma_buffer *dmab, | ||
| int direction) | ||
| { | ||
| /* the skl cl dma don't use stream tag, ret is for debug */ |
There was a problem hiding this comment.
please remove this func if its not needed.
| } | ||
|
|
||
| /* prepare DMA for code loader stream */ | ||
| ret = cl_stream_prepare_skl(sdev, 0x40, fwsize, &sdev->dmab, |
There was a problem hiding this comment.
magic number 0x40, please use a macro
2 participants
enable the kbl DSP core.
get the ROM init.